Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Core object-literal-key-quotes rule #1364

Merged
merged 3 commits into from
Jul 21, 2016
Merged

Conversation

danvk
Copy link
Contributor

@danvk danvk commented Jun 30, 2016

This adds a tslint equivalent to a subset of the eslint quote-props rule (see #1260).

The eslint rule is fairly elaborate. I've implemented the "as-needed" and "always" behaviors: http://eslint.org/docs/rules/quote-props

I verified that my tests produce the same results as eslint for the same code and settings.

There may be some incorrect behavior around things like unicode property names, but this is a good start.

@danvk
Copy link
Contributor Author

danvk commented Jul 12, 2016

@jkillian @adidahiya This is ready for review.

@jkillian
Copy link
Contributor

Sorry for the delay @danvk, was away from the office. I'll try to give this a review by the end of the week at the latest

/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
ruleName: "quote-props",
description: "Quoting Style for Property Names",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Enforces consistent object literal property quote style."

I think the above would be more consistent with the current descriptions.

@jkillian
Copy link
Contributor

Looking good! Just found a few documentation tweaks to make if you don't mind

@danvk
Copy link
Contributor Author

danvk commented Jul 20, 2016

Addressed all comments, thanks for the review!

}
}

// This is simplistic. See https://mothereff.in/js-properties for the gorey details.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol this url domain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole URL works pretty well for this topic! :)

@adidahiya
Copy link
Contributor

@danvk small note for future PRs: it makes it easier for us to review if you address CR comments in a follow-up commit (Github's squash+merge feature makes it easy to squash revision commits at the end). Thanks again for the contribution!

@danvk
Copy link
Contributor Author

danvk commented Jul 20, 2016

👍 @adidahiya

(I often use tools like Reviewable, which have a notion of revisions/snapshots independent from the current commits.)

@danvk
Copy link
Contributor Author

danvk commented Jul 20, 2016

(split into two commits so you guys can see the diff more easily.)

@jkillian
Copy link
Contributor

Thanks! Last change, could we rename the rule to object-literal-key-quotes? This will keep it parallel to an existing rule object-literal-sort-keys. Sorry for not mentioning this last time

@jkillian jkillian changed the title Core quote-props rule Core object-literal-key-quotes rule Jul 20, 2016
@danvk
Copy link
Contributor Author

danvk commented Jul 20, 2016

@jkillian do you guys care about maintaining parity with eslint? That's where the quote-props name comes from: http://eslint.org/docs/rules/quote-props

@jkillian
Copy link
Contributor

@danvk That's definitely something that's taken into consideration. However, I feel like ESLint rule names can be a little bit hit or miss, so I'm leaning away from worrying about parity too much. I'd like to get better naming guidelines formalized (#1371), so if you have overall input, would be glad to hear your thoughts.

@danvk
Copy link
Contributor Author

danvk commented Jul 20, 2016

object-literal-key-quotes it is!

@danvk
Copy link
Contributor Author

danvk commented Jul 20, 2016

@jkillian this should be good to go

@jkillian
Copy link
Contributor

Thanks @danvk, merged!

soniro pushed a commit to soniro/tslint that referenced this pull request Aug 31, 2016
* Core quote-props rule

* Tweaks for code review

* rename
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants